Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Assets Pallet: reintroduce fungibles::Destroy trait #12690

Closed

Conversation

tonyalaribe
Copy link
Contributor

This PR is an offshoot of #12310 , to maintain a way for users of the asset pallet still remove fungible assets.
It's a separate PR to allow it be reviewed easily without bloating #12310 further. But they should be both merged in together.

tonyalaribe and others added 30 commits September 20, 2022 11:59
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
…has begun the process; Add live check for other extrinsics
/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
///
/// Each call Emits the `Event::DestroyedAccounts` event.
fn destroy_accounts(id: Self::AssetId, max_items: u32) -> Result<u32, DispatchError>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left wondering what the u32 returned is. Is it the amount left or destroyed? Be good to be explicit about what it's returning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. I'll add it to the comment.

Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from already mentioned typo.

fn get_destroy_witness(id: &Self::AssetId) -> Option<Self::DestroyWitness>;

/// Destroy an existing fungible asset.
/// Start the destruction an existing fungible asset.
/// * `id`: The `AssetId` to be destroyed.
/// * `witness`: Any witness data that needs to be provided to complete the operation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no witness arg anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this. I've taken it out.

@@ -281,9 +275,41 @@ pub trait Destroy<AccountId>: Inspect<AccountId> {
///
/// If successful, this function will return the actual witness data from the destroyed asset.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part of the doc also needs to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Thanks again. Also taken it out now.

/// asset is in a `Destroying` state
///
/// Due to weight restrictions, this function may need to be called multiple
/// times to fully destroy all accounts. It will destroy `RemoveItemsLimit` accounts at a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no RemoveItemsLimit in this context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've updated the docs. The max_items value does the job in this case. Thanks for spotting it.

/// time.
///
/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc for max_items is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i've added that now. Thanks

///
/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
///
/// Each call Emits the `Event::DestroyedAccounts` event.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't know about the event in this context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I wasn't sure how to word it. But i've removed the reference.

Copy link
Contributor

@muharem muharem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the docs, the docs from the implementation is not relevant for the trait

@tonyalaribe tonyalaribe added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 14, 2022
witness: Self::DestroyWitness,
maybe_check_owner: Option<AccountId>,
) -> Result<Self::DestroyWitness, DispatchError>;
/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor note, the formatting is different, with what you have above, I think the rust docs can be used as a reference https://doc.rust-lang.org/rust-by-example/meta/doc.html

I also might be important because polkadot js displays the docs in UI, but not sure what it exactly expects

Base automatically changed from aa/safely-destroy-large-assets to master November 15, 2022 09:59
@jsidorenko
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Error: Command 'Command { std: "git" "merge" "origin/master" "--no-ff" "--no-edit", kill_on_drop: false }' failed with status Some(1); output: no output

@tonyalaribe
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Branch is already up-to-date

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants